Skip to content

Add Fermyon Spin adapter (wasm32-wasip1)#735

Open
prk-Jr wants to merge 27 commits into
feature/edgezero-pr18-phase5-verificationfrom
feature/edgezero-pr19-spin-adapter
Open

Add Fermyon Spin adapter (wasm32-wasip1)#735
prk-Jr wants to merge 27 commits into
feature/edgezero-pr18-phase5-verificationfrom
feature/edgezero-pr19-spin-adapter

Conversation

@prk-Jr

@prk-Jr prk-Jr commented May 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Introduces trusted-server-adapter-spin — the Fermyon Spin entry point (wasm32-wasip1 component) using edgezero_adapter_spin::run_app with Spin-backed RuntimeServices (config store, KV, secrets, buffered proxy client with gzip/br decompression)
  • Extends parity tests to cover Spin as a third in-process adapter alongside Fastly and Cloudflare
  • Bumps EdgeZero rev to ce6bcf74 (adds edgezero-adapter-spin), Fastly SDK to 0.12, Cloudflare worker to 0.8, and Viceroy to 0.16.5 to resolve bot-analysis hostcall breakage introduced by the SDK bump

Changes

File Change
crates/trusted-server-adapter-spin/ New crate: Spin entry point, edgezero.toml, spin.toml, platform runtime services, route/auth smoke tests, EdgeZero manifest validation test
Cargo.toml / Cargo.lock Add trusted-server-adapter-spin; bump EdgeZero rev, fastly 0.12, log-fastly 0.12, worker 0.8
crates/integration-tests/Cargo.toml / tests/parity.rs Extend parity suite to assert Spin adapter route behaviour matches Fastly and Cloudflare
.cargo/config.toml Add test-spin, check-spin, clippy-spin-native, clippy-spin-wasm aliases
.github/workflows/test.yml Add Spin native and wasm32-wasip1 CI jobs
.github/workflows/format.yml Add Spin clippy job
.github/actions/setup-integration-test-env/action.yml Install Viceroy 0.16.5
.tool-versions Pin Viceroy to 0.16.5
CLAUDE.md Document Spin commands and target-matched lint guidance
crates/trusted-server-adapter-fastly/src/{main,platform,route_tests}.rs Minor updates required by fastly 0.12 API surface
crates/trusted-server-adapter-cloudflare/Cargo.toml Bump worker to 0.8
docs/superpowers/specs/2026-03-19-edgezero-migration-design.md Update migration design notes for Spin phase

Closes

Closes #732

Known MVP limits

  • Spin component variables don't support hyphens, dots, or uppercase in key names — some Trusted Server config keys require manual mapping; authenticated key rotation is not claimed
  • Spin KV TTL is unavailable in the current EdgeZero Spin KV adapter
  • cargo build --workspace intentionally fails (mixed wasm32-wasip1 / wasm32-unknown-unknown targets); target-matched aliases are the correct verification path

Test plan

  • cargo fmt --all -- --check
  • cargo check (native)
  • cargo check-spin (wasm32-wasip1)
  • cargo build --package trusted-server-adapter-spin --target wasm32-wasip1 --features spin --release
  • cargo test-spin (route/auth smoke tests, native host)
  • cargo test-fastly (Viceroy 0.16.5)
  • cargo test-axum
  • cargo test-cloudflare
  • cargo test --manifest-path crates/integration-tests/Cargo.toml --test parity (Fastly + Cloudflare + Spin)
  • cargo clippy-fastly, clippy-axum, clippy-cloudflare, clippy-spin-native, clippy-spin-wasm
  • cargo clippy --manifest-path crates/integration-tests/Cargo.toml -- -D warnings
  • cargo check -p trusted-server-adapter-fastly --target wasm32-wasip1
  • cargo check -p trusted-server-adapter-cloudflare --target wasm32-unknown-unknown --features cloudflare
  • git diff --check
  • Other: spin up --from crates/trusted-server-adapter-spin (requires local Spin CLI install)

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses log macros (not println!)
  • New code has tests
  • No secrets or credentials committed

prk-Jr added 3 commits May 22, 2026 19:57
Adds trusted-server-adapter-spin: Spin entrypoint via
edgezero_adapter_spin::run_app, EdgeZero and Spin runtime manifests,
platform runtime services (null geo, sync Spin variable secret adapter,
conservative buffered HTTP client with gzip/br decompression policy),
route/auth smoke tests, and EdgeZero manifest validation test.

Bumps EdgeZero deps to ce6bcf74b529d9066d08ba87b2971af8379eb29e to
access edgezero-adapter-spin. The new rev requires fastly = "0.12"
(edgezero-adapter-fastly workspace dependency) and worker = "0.8"
(Cloudflare adapter type compatibility). Pins viceroy 0.16.5 to
resolve the Fastly SDK 0.12 bot-analysis hostcall issue.

Extends parity tests to include Spin as a third in-process adapter.
Adds target-matched cargo aliases and CI jobs for Spin native and
wasm32-wasip1 targets. Updates CLAUDE.md lint and build guidance for
the mixed-runtime workspace.

Known MVP limits: Spin component variables do not map cleanly to all
Trusted Server config keys; authenticated key rotation success is not
claimed. Spin KV TTL is unavailable in the current EdgeZero Spin KV
adapter.
@prk-Jr prk-Jr self-assigned this May 25, 2026
@prk-Jr prk-Jr requested review from ChristianPavilonis and aram356 and removed request for aram356 May 25, 2026 13:08
prk-Jr added 4 commits May 27, 2026 17:12
KIDs starting with a digit would alias in the Spin variable encoder
(both "1foo" and "n1foo" map to "v_n1foo"). Blocking them at validation
eliminates the risk without changing the encoding scheme.
- Filter WASI HTTP P2 forbidden outbound headers (host, connection,
  keep-alive, transfer-encoding, upgrade, proxy-connection) before
  building the Spin outbound request; the host header caused every
  proxy request to fail with HeaderError::Forbidden
- Fix push_spin_variable_escape to emit _xhh not _hh; corrects all
  variable names in spin.toml and encoded test expectations
- Prepend n to digit-leading keys in spin_variable_name so Spin's
  letter-led segment requirement is met; aliasing now unreachable
  because validate_kid rejects digit-leading KIDs
- Fix with_capacity to account for the optional n prefix
- Remove redundant localhost entries from allowed_outbound_hosts
  (http://*:* already covers them)
- Update spin.toml encoding comment: collision-free → collision-resistant
Resolve conflict in parity.rs: preserve axum_www_auth binding from base
branch (required by assert_eq! below) and move spin WWW-Authenticate
presence check to end of function, consistent with deactivate test pattern.
Spin's WASI HTTP bridge does not surface the incoming Host header via
IncomingRequest::headers() — the authority is only accessible through
Spin's spin-full-url synthetic header. EdgeZero's into_core_request
forwards all headers including spin-full-url, but the Host header is
absent. extract_request_host() returns "" which causes
classify_response_route to fall back to BufferedUnmodified, skipping
the HTML processor entirely: no TSJS injection, no URL rewriting, no
GTM proxying.

Inject Host from spin-full-url in dispatch before routing, via a
simple host_from_spin_url helper that strips the scheme and path. The
fix is Spin-specific and does not touch shared publisher code.

Also addresses PR review findings: remove build_per_request_services
passthrough wrapper, reduce MAX_DECOMPRESSED_SIZE to 8 MiB, annotate
streaming body limitation, expand validate_kid digit test, upgrade
parity WWW-Authenticate assertions from presence to value equality,
and document the anyhow exception at the WASM FFI boundary.

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Adds a Fermyon Spin entry-point crate (wasm32-wasip1 component) with an EdgeZero manifest, platform runtime services, conservative response policy (hop-by-hop sanitization, gzip/br decompression with a 64 MiB cap), route/auth smoke tests, and a three-way parity test extension. Bumps the EdgeZero rev plus fastly 0.12, worker 0.8, and Viceroy 0.16.5 to track the new SDK surface.

Overall the adapter is well-tested with documented MVP limits, but the Spin variable encoder makes safety claims it doesn't enforce, and outbound HTTP has no timeout. Requesting changes on those two items; the rest are non-blocking suggestions.

Blocking

🔧 wrench

  • spin_variable_name contract is broader than its safety claim: the comment + test name anchor on validate_kid, but the encoder is called by config and secret stores with arbitrary keys. Two reachable foot-guns — digit-leading aliasing and adjacent-underscore invalidity for uppercase/_/-/./: -leading keys. Fix by rejecting at the encoder boundary. (crates/trusted-server-adapter-spin/src/platform.rs:138-176)
  • No outbound HTTP timeout on Spin: spin_sdk::http::send is called with no timeout; NoopBackend discards PlatformBackendSpec::first_byte_timeout. Either honor it via a race-with-delay future or document the limitation explicitly. (crates/trusted-server-adapter-spin/src/platform.rs:399-444)

Non-blocking

♻️ refactor

  • startup_error_router skips AuthMiddleware and methods beyond GET/POST — the fallback router leaks startup error text on admin routes without a 401 challenge, and non-GET/POST methods bypass the error message entirely. (crates/trusted-server-adapter-spin/src/app.rs:128-154)
  • Duplicate /first-party/sign handlersfp_sign_get_handler and fp_sign_post_handler are byte-identical closures. (crates/trusted-server-adapter-spin/src/app.rs:264-288)

🤔 thinking

  • Multi-provider fan-out rejected at request time onlyselect returns an error when 2+ providers are submitted, but operators won't discover this until /auction traffic hits. A startup-time warning when adapter + provider count are incompatible would surface the limit sooner. (crates/trusted-server-adapter-spin/src/platform.rs:507-547)
  • SpinSecretStoreAdapter UTF-8 and plaintext constraints — Spin variables are UTF-8 strings and unencrypted in the manifest by default. Today's JSON-encoded signing keys work; binary secrets would silently break, and production deployments must back the variables with a real secret-provider source. Worth a # Limitations rustdoc section. (crates/trusted-server-adapter-spin/src/platform.rs:570-598)

⛏ nitpick

  • with_capacity under-allocates — escapes are 4 bytes per char; current bound assumes 1:1. (platform.rs:150)
  • Test name overpromisesspin_variable_name_digit_prefix_aliasing_is_unreachable_for_valid_kids is true only for callers that pre-validate. Pair with an encoder-level test or rename. (platform.rs:980)

🌱 seedling

  • No live Spin-runtime CI gate — the PR test plan's spin up --from ... is intentionally unchecked. Today's CI builds and runs native-host route tests; a follow-up could exercise the WASM artifact under a real Spin runtime (the analog of Viceroy for Fastly).

📌 out of scope

  • clippy-cloudflare added to format.yml — closes an existing gap but is orthogonal to the Spin adapter. Calling it out so it isn't lost.

CI Status

  • fmt: PASS
  • clippy (Fastly / Axum / Cloudflare / Spin native + wasm32-wasip1): PASS
  • cargo test (Fastly via Viceroy): PASS
  • cargo test (Axum native): PASS
  • cargo test (Cloudflare native): PASS
  • cargo check/build (Spin native + wasm32-wasip1): PASS
  • cargo test (cross-adapter parity): PASS
  • vitest: PASS
  • format-docs / format-typescript: PASS
  • integration + browser tests: PASS

Comment thread crates/trusted-server-adapter-spin/src/platform.rs
Comment thread crates/trusted-server-adapter-spin/src/platform.rs
Comment thread crates/trusted-server-adapter-spin/src/app.rs
Comment thread crates/trusted-server-adapter-spin/src/app.rs Outdated
Comment thread crates/trusted-server-adapter-spin/src/platform.rs
Comment thread crates/trusted-server-adapter-spin/src/platform.rs
Comment thread crates/trusted-server-adapter-spin/src/platform.rs Outdated
Comment thread crates/trusted-server-adapter-spin/src/platform.rs Outdated

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Submitting the review findings as a comment review. CI is passing. I did not duplicate the existing inline review comments for the Spin variable encoder, missing outbound timeout, or startup fallback because those are already covered by an existing changes-requested review on this PR.

The additional findings below are folded into the review body because their referenced lines are outside this stacked PR's current GitHub diff, so GitHub cannot attach them as inline comments on PR #735.

Additional findings

P1 — proxy-rebuild can mint valid click redirects without validating the original token

crates/trusted-server-core/src/proxy.rs:1107

handle_first_party_proxy_rebuild extracts tsurl, ignores any supplied tstoken, then signs a new /first-party/click URL. Because this route is public, an attacker can create valid click redirects to arbitrary URLs and, when a victim follows them, handle_first_party_click may append the victim's EC ID to the attacker-controlled redirect target.

Suggestion: Validate payload.tsclick with reconstruct_and_validate_signed_target before applying changes, reject missing/invalid tstoken, and enforce settings.proxy.allowed_domains in the click redirect path before returning Location.

P2 — Cloudflare integration build uses stale SYNTHETIC env var

.github/actions/setup-integration-test-env/action.yml:132

The Cloudflare build step sets TRUSTED_SERVER__SYNTHETIC__SECRET_KEY, but the current settings field is edge_cookie.secret_key. The override is ignored, so the Cloudflare integration artifact is built with the default config value instead of the intended integration-test secret.

Suggestion: Replace it with:

TRUSTED_SERVER__EDGE_COOKIE__SECRET_KEY: integration-test-secret-key

P3 — Public docs/README do not reflect Spin or target-matched commands

docs/guide/architecture.md:7, README.md:51

The public docs still describe only Fastly/Axum runtimes and recommend broad workspace clippy commands that the project now documents as problematic for mixed runtimes.

Suggestion: Update README and guide pages to include Spin commands (cargo test-spin, cargo check-spin, the Spin build command) and target-matched clippy aliases.

Existing findings already covered by prior review

  • P1 — Spin variable encoder accepts unsafe key shapes (crates/trusted-server-adapter-spin/src/platform.rs).
  • P1 — Spin outbound HTTP ignores configured timeouts (crates/trusted-server-adapter-spin/src/platform.rs).
  • P2 — Spin startup fallback bypasses admin auth and only handles GET/POST (crates/trusted-server-adapter-spin/src/app.rs).

prk-Jr added 6 commits May 29, 2026 15:24
Blocking:
- Reject keys not starting with a lowercase ASCII letter at the
  spin_variable_name encoder boundary; removes the aliasing-prone
  digit-leading n-prefix branch and fixes with_capacity to worst-case
  (key.len() * 4 + 3). Updates affected tests to assert rejection.
- Document the no-configurable-outbound-timeout limitation in
  SpinPlatformHttpClient rustdoc; PlatformBackendSpec::first_byte_timeout
  is ignored by NoopBackend and Spin's http::send has no per-request
  timeout API.

Non-blocking:
- startup_error_router now logs the error and returns a generic
  "Service Unavailable" body so deployment state is not leaked to
  anonymous callers; adds PUT/DELETE handlers for all catch-all routes.
- Collapse duplicate fp_sign_get_handler/fp_sign_post_handler into one
  closure + clone, matching the get_fallback/post_fallback pattern.
- Add # Limitations rustdoc section to SpinSecretStoreAdapter covering
  UTF-8-only variables and plaintext-at-rest in the default manifest.

Security (P1 — proxy-rebuild):
- handle_first_party_proxy_rebuild now validates the tstoken on the
  original tsclick URL via reconstruct_and_validate_signed_target before
  applying any parameter mutations. Without this an attacker could submit
  an unsigned tsclick and mint valid click redirects to arbitrary URLs.
- Enforce settings.proxy.allowed_domains on tsurl before issuing the
  new signed redirect.

CI fix (P2):
- Replace stale TRUSTED_SERVER__SYNTHETIC__SECRET_KEY env var with
  TRUSTED_SERVER__EDGE_COOKIE__SECRET_KEY in the Cloudflare integration
  build step; the previous key was ignored, leaving the artifact built
  with the default config value.
- README Quick Start: add cargo test-spin
- README Development: replace broad workspace clippy with target-matched
  aliases and note why; add cargo test-spin to test list
- architecture.md: mention Cloudflare/Spin in the high-level overview;
  add trusted-server-adapter-spin section with build/test/lint commands
  and known MVP limits; expand Runtime Targets table to include all four
  adapters; add note on target-matched clippy requirement
Test was constructing a tsclick URL without a signature. Now that
handle_first_party_proxy_rebuild validates the tstoken via
reconstruct_and_validate_signed_target, the test must supply a properly
signed click URL. Compute the token with compute_encrypted_sha256_token
over tsurl + original params before building tsclick.
- Collapse nested if/if-let/if-let into a single let-chain condition
  (collapsible_if, Rust 2024 edition let chains)
- Move host_from_spin_url test module to end of file so no items follow
  the test module (items_after_test_module)
Conflict resolutions:
- .cargo/config.toml: keep PR18's clippy-cloudflare without
  --all-features (the cloudflare feature has a non-wasm32
  compile_error! guard) alongside PR19's spin aliases
- CLAUDE.md / README.md: keep PR19's per-adapter lint command list;
  drop the broad --all-features workspace clippy suggestion that the
  cloudflare guard would trip
- cloudflare build.sh: keep PR19's worker-build ^0.8 (matches the
  worker 0.8 dependency bump)
- integration-tests/Cargo.toml: keep PR19's edgezero rev ce6bcf74
  (matches the bumped root workspace pin) with PR18's dual-bump
  warning comment, PR19's exact tokio pin, and PR18's urlencoding
- core proxy.rs: keep PR18's #[test] + block_on style (core has no
  tokio, tests must run on wasm via Viceroy) with PR19's updated
  proxy_rebuild test body that signs the click URL for tstoken
  validation
- parity.rs: keep PR19's three-way Spin coverage on top of PR18's
  routes_with_settings seams; spin_router() helper added; JSON
  key-set parity extended to Spin
- Lock files regenerated from the merged manifests

Semantic fixes for PR16-18 API changes in the Spin adapter:
- handle_proxy now takes ProxyDispatchInput
- handle_auction gained kv, partner registry, and EC context parameters
- PlatformSelectResult gained failed_backend_name
- supports_concurrent_fanout() reports false (send_async executes
  eagerly) so the orchestrator rejects multi-provider auctions before
  any request launches; select keeps its defense-in-depth rejection
- resolve_publisher_response delegates to the shared
  buffer_publisher_response so the max_buffered_body_bytes cap applies
  on Spin too
- TrustedServerApp::routes_with_settings() seam added; Spin middleware
  and route tests build explicit test settings instead of the baked
  placeholder secrets that get_settings() rejects

Also adapt fastly main.rs TLS metadata calls to the fastly 0.12 API
(get_tls_protocol / get_tls_cipher_openssl_name now return
Result<Option<&str>>)
The merged [lints.clippy] block from PR18 now applies to this branch's
pre-existing integration test files:
- Collapse the nested if in is_ec_cookie_expired into a let-chain
- Replace redundant closures with serde_json::Value::as_u64
- Replace the denied panic! in the EC scenario loop with an assert!
  carrying the same diagnostic message

CI's freshly installed worker-build ^0.8 requires wasm-bindgen 0.2.123
or newer, but the lock files held worker 0.8.3 with wasm-bindgen
0.2.121 (js-sys exact-pins its companion wasm-bindgen, so only the
worker 0.8.4 bump moves it). Update worker in both the workspace and
integration-tests lock files.

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review: I reviewed PR #735 against feature/edgezero-pr18-phase5-verification, focusing on the new Spin adapter, the core proxy/signing deltas, and the adapter parity/CI changes. CI is green, and the earlier review items for the Spin variable encoder, outbound timeout documentation, startup fallback leak, and unsigned proxy-rebuild have been addressed.

I found several issues that should block merging: the Spin adapter does not preserve trusted request scheme/sanitize spoofable forwarded headers for publisher/integration rewriting, its route table does not match the Fastly/Axum publisher fallback methods for non-GET/POST requests, and proxy-rebuild can strip the tsexp replay bound from a signed click URL and re-sign it indefinitely. I also left one non-blocking compatibility note about digit-leading legacy KIDs.

CI / Existing Reviews:

  • gh pr checks 735: all reported checks pass (fmt, Rust tests, cross-adapter parity, Spin native+wasm build/check, Cloudflare checks, vitest, docs/TS formatting, integration/browser tests).
  • Existing reviews already covered and the current HEAD appears to have fixed the prior Spin variable encoder, timeout documentation, startup fallback, Cloudflare env var, docs, and initial proxy-rebuild validation findings.

Comment thread crates/trusted-server-adapter-spin/src/app.rs Outdated
Comment thread crates/trusted-server-adapter-spin/src/app.rs Outdated
Comment thread crates/trusted-server-core/src/proxy.rs
Comment thread crates/trusted-server-core/src/request_signing/endpoints.rs Outdated
prk-Jr added 6 commits June 13, 2026 10:47
Conflict resolutions:
- CLAUDE.md / README.md: kept the spin-aware clippy aliases (clippy-spin-native,
  clippy-spin-wasm) and the build-alias note from the base.
- integration.rs: both branches replaced the panic with an assert; kept the
  base's "should succeed" wording.
- parity.rs: combined PR18's stronger assertions with PR19's three-adapter
  coverage — full parsed-body equality for the discovery JSON (incl. spin),
  /auction status equality plus non-404 across axum/cf/spin, and canonical
  /_ts/admin/keys/* paths for the admin auth parity tests.

To make the merged admin-auth contract hold on Spin (which previously only
registered legacy /admin/keys/*), registered the canonical /_ts/admin/keys/*
routes on the Spin adapter and aligned its route tests to the production-shaped
^/_ts/admin handler regex, mirroring the PR18 fix for the Axum and Cloudflare
adapters.
The Spin fallback dispatch reconstructed only the Host header from the trusted
spin-full-url and left client-supplied Forwarded/X-Forwarded-* headers intact.
Because build_runtime_services sets no TLS signal, detect_request_scheme had no
trusted scheme source and fell back to http, or to a client-spoofed scheme/host,
which publisher HTML rewriting, integration URL rewriting, and request-signing
context then consumed.

Strip the spoofable forwarded headers (mirroring the Fastly/Axum edge
sanitization), then inject both the trusted Host and the trusted scheme parsed
from spin-full-url. Extracted the logic into apply_trusted_host_and_scheme and
added unit tests covering spoofed X-Forwarded-* headers and Host preservation.
The Spin route table only registered GET and POST for / and /{*rest} and only
each named route's primary method, so HEAD /, OPTIONS /page (CORS preflight),
HEAD /first-party/proxy, and similar requests returned a router-level 405
instead of reaching the publisher/integration fallback like Fastly and Axum.

Mirror the Fastly/Axum publisher_fallback_methods() pattern: register GET, POST,
HEAD, OPTIONS, PUT, PATCH, and DELETE for the catch-all and for every named
path's non-primary methods, all routed through the shared fallback. Dynamic
/static/tsjs= handling stays GET-only (now gated inside dispatch on the request
method). Added route tests for HEAD /, OPTIONS, and a non-primary method on a
named path; updated the PUT /verify-signature test to assert fall-through.
handle_first_party_proxy_rebuild validated the original tstoken but then let
payload.del/add mutate any parameter, including the reserved tsexp replay bound
that handle_first_party_proxy_sign attaches. A public rebuild request could take
a still-valid expiring /first-party/click URL, delete tsexp, and receive a
freshly signed click URL that reconstruct_and_validate_signed_target accepts
indefinitely (expiration is optional).

Treat tsexp, tstoken, and tsurl as reserved: reject del/add for them so the
bounded expiration is preserved and re-signed. Added regression tests asserting
deletion of tsexp is rejected and that a normal rebuild retains the tsexp bound.
The digit-leading KID rejection (added to block Spin variable-encoder aliasing)
was applied through validate_kid to every caller, including
handle_deactivate_key. Deployments that created operator-supplied digit-leading
KIDs under the previous validation rules could no longer deactivate or delete
them, since the request was rejected before storage was touched.

Split validation: validate_kid_format enforces the structural constraints
(length, charset) and validate_kid layers the digit-leading rejection on top for
creation/rotation. Deactivation and deletion now use validate_kid_format so
legacy digit-leading KIDs remain removable. Added tests covering both paths.
The Cloudflare build installed worker-build with a floating `^0.8`, which began
resolving to worker-build 0.8.5. That release passes `--force-enable-abort-handler`
to wasm-bindgen, but worker-build downloads the wasm-bindgen CLI matching the
locked `wasm-bindgen` (0.2.123, pulled by worker 0.8.4), and that CLI does not
recognise the flag — so the integration-test Cloudflare bundle build failed with
"unexpected argument '--force-enable-abort-handler'".

Derive the worker crate version from Cargo.lock and pin worker-build to it.
worker-build is released in lockstep with worker, so this keeps the build tool
and the wasm-bindgen CLI it drives compatible, and the pin advances automatically
when the lockfile bumps worker. Verified locally: build.sh installs worker-build
0.8.4 and produces build/index.js without error.

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review: I reviewed PR #735 against feature/edgezero-pr18-phase5-verification, focusing on the current Spin adapter head, the prior review fixes, route/header behavior, request-signing/proxy changes, and CI/test coverage. CI is currently green and the earlier blocking review items appear to have been addressed, but I found one remaining blocking Spin correctness issue: the first-party proxy/click/sign path still receives path-only Spin URIs while the shared core handlers parse absolute URLs. That makes the creative first-party proxy flow fail on Spin. I also left two non-blocking compatibility findings for startup fallback method coverage and KID validation consistency.

CI / Existing Reviews:

  • gh pr checks 735: all reported checks pass (fmt, Rust tests, cross-adapter parity, Spin native + wasm32-wasip1 check/build, Cloudflare checks, vitest, docs/TS formatting, integration/browser tests).
  • Existing review feedback already covered the Spin variable encoder, outbound timeout docs, trusted scheme/header sanitization in publisher fallback, route fallback methods, proxy-rebuild signing controls, legacy digit-leading KID removal, Cloudflare worker-build pinning, docs, and startup error leak. I avoided duplicating resolved items except where the current code still leaves a distinct edge case.

Comment thread crates/trusted-server-adapter-spin/src/app.rs Outdated
Comment thread crates/trusted-server-adapter-spin/src/app.rs Outdated
Comment thread crates/trusted-server-core/src/request_signing/endpoints.rs
prk-Jr added 2 commits June 13, 2026 16:29
Spin builds the core request URI from IncomingRequest::path_with_query(),
so it is path-only (e.g. "/first-party/proxy?..."). The shared first-party
proxy/click/sign handlers parse req.uri() with url::Url::parse, which
rejected the relative path with "Invalid URL" — breaking the creative
first-party proxy flow, click redirects, and GET sign query parsing on Spin.

Rename apply_trusted_host_and_scheme to normalize_spin_request and have it
rebuild an absolute URI from the trusted spin-full-url scheme+host, then
apply it to every named first-party handler (proxy, click, sign GET+POST,
proxy-rebuild) in addition to the publisher fallback.

Also build the startup error router from the full publisher fallback method
set so HEAD, OPTIONS, and PATCH on "/" and nested paths return the generic
503 instead of a router-level 405, matching the healthy router.

Add regression tests: a signed proxy round-trip and GET sign query parse
through the Spin router, plus a startup-fallback method-coverage unit test.
validate_kid now requires a lowercase ASCII leading character, matching the
strictest platform key-name encoder (the Spin variable encoder, which rejects
uppercase- and punctuation-leading KIDs and aliases digit-leading ones). A KID
minted on any adapter must be storable on every backend, so create/rotate
validates against the strictest common denominator and returns a client-
correctable 400 instead of a runtime 5xx on Spin. Deactivation keeps the looser
format check so legacy KIDs stay removable.

Expose kid_is_creatable and add a Spin contract test asserting the encoder
accepts every creatable KID, pinning core >= encoder strictness so the two
lowercase-leading rules cannot silently drift.

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review: I reviewed PR #735 against feature/edgezero-pr18-phase5-verification, focusing on the current Spin adapter implementation, request normalization/routing parity, Spin platform services, core proxy/signing changes, and CI/tooling updates. I did not find a blocking correctness, security, data-loss, authorization, or severe compatibility issue, so this automated workflow is submitting COMMENT rather than REQUEST_CHANGES.

I left inline comments for one high-priority Spin response-size/resource-safety issue and a few non-blocking compatibility/tooling gaps.

CI / Existing Reviews:

  • gh pr checks 735: all reported checks pass (fmt, Rust tests, cross-adapter parity, Spin native + wasm32-wasip1 check/build, Cloudflare checks, vitest, docs/TS formatting, integration/browser tests).
  • Existing review threads on the Spin variable encoder, outbound timeout documentation, startup fallback, first-party request URI normalization, route fallback methods, proxy-rebuild signing controls, legacy KID removal, and worker-build pinning appear to have been addressed at the current head.

Comment thread crates/trusted-server-adapter-spin/src/platform.rs
Comment thread crates/trusted-server-adapter-spin/src/app.rs Outdated
Comment thread crates/trusted-server-adapter-spin/src/app.rs
Comment thread .cargo/config.toml Outdated
prk-Jr added 3 commits June 13, 2026 21:27
The 8 MiB ceiling was only applied after decompressing a gzip or br body.
An identity response, or an unsupported encoding such as zstd, returned the
fully buffered Vec<u8> with no size check, so a large origin/ad-server/proxy
response could still be forwarded after exhausting the Spin WASM heap budget
instead of failing with a typed error.

apply_spin_response_policy now bounds the buffered body on every path via
enforce_response_size before returning. For gzip/br this also rejects an
overlarge compressed body before decompression, while decompress_body keeps
bounding the expanded size. Add tests for an oversized identity body, an
oversized unsupported-encoding body, and an identity body exactly at the limit.
normalize_spin_request now always sets Host from the trusted spin-full-url
host instead of preserving a client-supplied value. Keeping an incoming Host
while rebuilding req.uri() from the spin-full-url host left the shared
RequestInfo path (publisher HTML rewriting, integration URL rewriting, signing
context) reading one authority while handlers parsing req.uri() saw another.
Overriding from the single trusted authority keeps both consistent and drops a
client-spoofing vector. The unit test now asserts replacement.

Also add GET /health returning 200 "ok" to both the healthy router and the
startup-error fallback, matching the Fastly entry point and Axum adapter so
deployments can reuse the same liveness probe on Spin. Spin's platform-provided
/.well-known/spin/health is left untouched. Add route and startup-fallback
health tests.
These workspace aliases excluded only Axum and Cloudflare, so after adding
trusted-server-adapter-spin as a member they compiled the Spin adapter on
wasm32-wasip1 without the spin feature. Match the same split already used by
test-fastly and clippy-fastly and rely on the Spin-specific aliases/jobs for
Spin coverage.

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review: I reviewed PR #735 against feature/edgezero-pr18-phase5-verification, focusing on the new Fermyon Spin adapter, request normalization/routing, request-signing/admin endpoints, Spin platform services, parity tests, and CI/tooling updates.

I found one authorization issue that should block merge: the new Spin router exposes legacy /admin/keys/* admin aliases, but the production-shaped handler regex and Settings::ADMIN_ENDPOINTS only cover /_ts/admin/*, so unauthenticated requests can reach the admin handlers on those aliases. I also left one non-blocking privacy/compatibility note about forwarding Spin runtime synthetic headers to publisher origins.

CI / Existing Reviews:

  • gh pr checks 735: all reported checks pass (fmt, Rust tests, cross-adapter parity, Spin native + wasm32-wasip1 check/build, Cloudflare checks, vitest, docs/TS formatting, integration/browser tests).
  • Existing review threads on the Spin variable encoder, outbound timeout docs, startup fallback, first-party URI normalization, fallback methods, proxy-rebuild signing controls, legacy KID removal, response-size limits, Host normalization, health route, and worker-build/alias fixes appear addressed at the current head.

Comment thread crates/trusted-server-adapter-spin/src/app.rs Outdated
Comment thread crates/trusted-server-adapter-spin/src/app.rs
prk-Jr added 2 commits June 14, 2026 11:25
…aders

The Spin router registered legacy `/admin/keys/*` aliases as admin routes,
but the production basic-auth handler regex `^/_ts/admin` does not match
them, so AuthMiddleware never challenged those paths and unauthenticated
callers could reach the key rotate/deactivate handlers. Remove the aliases;
unrouted, they fall through to the publisher fallback like any unknown path.
The canonical `/_ts/admin/keys/*` routes stay auth-gated.

Also strip Spin's synthetic `spin-full-url` and `spin-client-addr` headers
in normalize_spin_request. They are runtime metadata whose parsed values are
already available via SpinRequestContext, and leaving them on the request let
the publisher fallback forward them to publisher origins. spin-full-url is
removed as it is consumed; spin-client-addr is removed unconditionally.

Add a parity regression test asserting the legacy aliases are not auth-gated
admin routes and behave like an unknown publisher-fallback path, and extend
the normalize test to assert both synthetic headers are stripped.
…/edgezero-pr19-spin-adapter

# Conflicts:
#	crates/integration-tests/tests/parity.rs

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review: I reviewed PR #735 against feature/edgezero-pr18-phase5-verification, focusing on the current Spin adapter request normalization, routing/auth behavior, Spin platform services, parity coverage, and the prior review fixes. CI is green, but I found two remaining security/correctness issues that should block merging: the adapter derives trusted host/scheme from a spoofable spin-full-url header path, and the auction route bypasses Spin normalization entirely so forwarded-header spoofing can still affect request-signing metadata. I also left one non-blocking compatibility finding for missing EC API route coverage on Spin.

CI / Existing Reviews:

  • gh pr checks 735: all reported checks pass (fmt, Rust tests, cross-adapter parity, Spin native + wasm32-wasip1 check/build, Cloudflare checks, vitest, docs/TS formatting, integration/browser tests).
  • Existing review threads on the Spin variable encoder, outbound timeout docs, startup fallback, first-party URI normalization, fallback methods, proxy-rebuild signing controls, legacy KID removal, response-size limits, Host normalization, health route, worker-build/alias fixes, and legacy admin aliases appear addressed at the current head. These comments avoid duplicating those resolved issues.

// trusted scheme/host; `spin-client-addr` carries no further use on the request.
req.headers_mut().remove("spin-client-addr");

let full_url = req.headers_mut().remove("spin-full-url");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review: P1 / High — spin-full-url is used as the trusted authority even though duplicate client-supplied headers can win.

This removes spin-full-url from the normal request header map and uses that value to overwrite Host, inject x-forwarded-proto, and rebuild req.uri(). The upstream EdgeZero Spin conversion copies all incoming headers and extracts spin-full-url from that same header list; Spin's HTTP trigger prepares component headers by appending synthetic spin-full-url after the original request headers. Because HeaderMap::remove returns the first value for duplicate names, a client-supplied spin-full-url can be selected before Spin's synthetic value. That lets an attacker choose the host/scheme consumed by publisher HTML rewriting, integration URL rewriting, and first-party signing/proxy validation context.

Suggested fix: don't derive trusted authority from a normal header lookup. Prefer fixing the EdgeZero Spin conversion to use IncomingRequest::uri() / scheme() / authority() (or a dedicated request extension populated before client headers are copied), strip client-supplied spin-* headers, and have this adapter consume only that trusted extension/URI. Add a regression test where a request carrying spin-full-url: https://evil.example/... cannot override the runtime authority.

let s = Arc::clone(&s);
async move {
let services = build_runtime_services(&ctx);
let req = ctx.into_request();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review: P1 / High — POST /auction bypasses Spin request normalization and still trusts spoofable forwarded headers.

The auction handler passes ctx.into_request() directly to handle_auction without calling normalize_spin_request or even sanitize_forwarded_headers. Prebid request-signing builds SigningParams from RequestInfo::from_request, and that helper intentionally prefers Forwarded / X-Forwarded-Host / X-Forwarded-Proto when the adapter has not stripped them. On Spin, ClientInfo also has no TLS signal, so a client can spoof the host/scheme used in downstream signed OpenRTB metadata.

Suggested fix: normalize/sanitize Spin requests before every handler that may call RequestInfo::from_request, including /auction (ideally via a middleware or shared handler wrapper rather than per-route calls). Add a Spin route/parity test for POST /auction with spoofed Forwarded/X-Forwarded-* headers proving those values are stripped and the trusted runtime authority is used.

// Named routes paired with the methods they handle directly. Every other
// publisher-fallback method on these paths is routed to the fallback so it
// reaches the publisher origin rather than a router-level 405.
fn named_fallback_paths() -> [(&'static str, &'static [Method]); 9] {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review: P2 / Medium — Spin omits the EC API routes that the Fastly EdgeZero path registers.

The Spin named-route table covers discovery, verification, admin, auction, and first-party proxy routes, but not POST /_ts/api/v1/batch-sync, GET /_ts/api/v1/identify, or OPTIONS /_ts/api/v1/identify. Fastly's EdgeZero router registers those EC routes, so on Spin these requests fall through to the publisher/integration fallback instead of the EC handlers; the cross-adapter parity suite currently does not cover this gap.

Suggested fix: either wire the Spin adapter to handle_batch_sync, handle_identify, and cors_preflight_identify with route tests/parity coverage, or explicitly document these EC API endpoints as unsupported for the Spin MVP and add tests that lock in the intentional degraded behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants